Conversation
WalkthroughThis PR adds two new configuration options to EloqGlobalOptions: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (1)
190-202: Ports parsing logic is sound; consider trimming whitespace as a nicetyThe comma‑separated parsing into
cloud_store_daemon_portsis correct and guarded on non‑empty input. If you expect admins to write values like"5572, 5573, 5574", you might optionally trim leading/trailing whitespace fromtokenbefore pushing toports, to keep the config canonical.- while (std::getline(tokenStream, token, ',')) { - if (!token.empty()) { - ports.emplace_back(token); - } - } + while (std::getline(tokenStream, token, ',')) { + // Trim spaces so "5572, 5573" works as expected. + auto first = token.find_first_not_of(" \t"); + if (first == std::string::npos) { + continue; + } + auto last = token.find_last_not_of(" \t"); + ports.emplace_back(token.substr(first, last - first + 1)); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/mongo/db/modules/eloq/src/eloq_global_options.cpp(4 hunks)src/mongo/db/modules/eloq/src/eloq_global_options.h(2 hunks)src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-14T03:10:22.027Z
Learnt from: githubzilla
Repo: eloqdata/eloqdoc PR: 279
File: src/mongo/db/dbmain.cpp:0-0
Timestamp: 2025-11-14T03:10:22.027Z
Learning: In this repository, avoid defining --config via gflags in src/mongo/db/dbmain.cpp; MongoDB’s options_parser owns --config. Prefer gflags flags with a distinct prefix (e.g., --data_substrate_config) parsed before options_parser, with options_parser configured to allow unregistered options.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
📚 Learning: 2025-11-24T03:11:24.955Z
Learnt from: lzxddz
Repo: eloqdata/eloqdoc PR: 325
File: src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp:1362-1384
Timestamp: 2025-11-24T03:11:24.955Z
Learning: The EloqDoc codebase uses an older version of glog that supports the two-parameter signature `google::InitGoogleLogging(const char* argv0, PrefixCallback)` for custom log prefix formatting, as seen in src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp line 1383.
Applied to files:
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp
🔇 Additional comments (7)
src/mongo/db/modules/eloq/src/eloq_kv_engine.cpp (2)
27-27:<sstream>include is appropriateNeeded for the new
std::istringstreamusage below; inclusion is correct and localized.
268-269:prewarm_cloud_cacheflag is correctly wired throughPropagating
eloqStorePrewarmCloudCachedirectly intoeloq_store_config.eloqstore_configs_.prewarm_cloud_cacheis consistent with the other EloqStore options here.src/mongo/db/modules/eloq/src/eloq_global_options.h (1)
121-150: New EloqStore option fields are well‑placed with safe defaultsAdding
eloqStorePrewarmCloudCache(defaultfalse) andeloqStoreCloudStoreDaemonPorts(empty string) in the EloqStore block keeps configuration cohesive and matches how they’re consumed instore()andconfigureEloqStore.src/mongo/db/modules/eloq/src/eloq_global_options.cpp (4)
428-433: Cloud store daemon ports option is declared consistentlyThe
storage.eloq.storage.eloqStoreCloudStoreDaemonPortsString option, with an empty default, lines up with the newstd::string eloqStoreCloudStoreDaemonPortsfield and the parsing logic inconfigureEloqStore.
583-588: Prewarm cloud cache option integrates cleanlyThe Bool option
storage.eloq.storage.eloqStorePrewarmCloudCache(defaultfalse) matches theeloqStorePrewarmCloudCachefield and is described clearly for users.
997-1000: Storing cloud daemon ports from params is straightforwardConditionally assigning
eloqStoreCloudStoreDaemonPortsfrom the corresponding parameter when present mirrors existing patterns and lets downstream code distinguish “unset” (empty string) from “set but empty” viaparams.count.
1089-1092: Prewarm flag storage follows existing Bool option conventionsThe
eloqStorePrewarmCloudCacheflag is stored only when the CLI/config option is present, with the default remainingfalseotherwise, which is consistent with other EloqStore Bool options.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.